-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEATURE] Add "ui5 use/add" commands #315
Conversation
|
Co-Authored-By: Merlin Beutlberger <m.beutlberger@sap.com>
Co-Authored-By: Merlin Beutlberger <m.beutlberger@sap.com>
lib/cli/commands/add.js
Outdated
type: "string" | ||
}).option("development", { | ||
describe: "Add as development dependency", | ||
alias: "D", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why uppercase? For the other commands, the shortcut has the same case as the option, as far as I can see.
I find consistency within our cli more appealing than consistency of a single command with yarn. Or can we have two aliases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this feature make it into the 2.0 release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeworrior - it is not only yarn - moreover its also consistency with npm:
https://docs.npmjs.com/cli/install
So, i personally like the abbreviation with the uppercase alias. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we looked at the aliases of the npm install
command here. However, I would personally prefer a --dev
and --opt
alias. We can add as many alias as we like 😏
Will this feature make it into the 2.0 release?
Yes, definitely. Our documentation already mentions it: https://github.com/SAP/ui5-tooling/pull/221/files#diff-1a523bd9fa0dbf998008b37579210e12R32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! So we just add all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll discuss our alias options with @matz3 in the morning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into yarn, they call --opt
=> --optional
-> I would try to align myself as much as possible with already existing CLIs. This helps people to easily use the one or the other. The uppercase aliases are common across npm and yarn and that is why I am also in favor to have them.
ui5 use
commandFollowup of #315. Add ui5 remove command which removes libraries from the framework libraries section in the ui5.yaml.
* [FEATURE] Add "ui5 remove" command Followup of #315. Add ui5 remove command which removes libraries from the framework libraries section in the ui5.yaml.
No description provided.